-
-
Notifications
You must be signed in to change notification settings - Fork 732
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for virtualenv #1119
base: main
Are you sure you want to change the base?
Conversation
Add a `gef.virtualenv_path` config var, and activate it at startup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR: I think this needs to be re-worked at least by adding way more test cases and documentation.
As I mentioned, a lot of complications come when dealing with seperate environments, and so it should be tested way more, for instance:
- we need to check the venv is running at least for the minimum Python version we supported
- what happens when a user uses a python version different from that with gdb
- what happens when a user switches between venv
- etc.
In addition, this should be exhaustively documented - not just the gef config
option, but how to use gef in a virtual env because it is 100% certain users will come with such questions (and/or bad issue reports about it)
The pyenv
was done hastily which came to bite us down the line, let's not repeat that mistake
if path: | ||
activate_script_path = pathlib.Path(path)/"bin"/"activate_this.py" | ||
exec(open(activate_script_path).read(), {'__file__': activate_script_path}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid cascading by returning first on error
if path: | |
activate_script_path = pathlib.Path(path)/"bin"/"activate_this.py" | |
exec(open(activate_script_path).read(), {'__file__': activate_script_path}) | |
if not path: | |
return | |
activate_script_path = pathlib.Path(path)/"bin"/"activate_this.py" | |
exec(open(activate_script_path).read(), {'__file__': activate_script_path}) |
if path: | ||
activate_script_path = pathlib.Path(path)/"bin"/"activate_this.py" | ||
exec(open(activate_script_path).read(), {'__file__': activate_script_path}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also simpler
exec(activate_script_path.read_text(), {'__file__': activate_script_path})
def load_virtualenv(self, new_path: Optional[pathlib.Path] = None): | ||
path = new_path or gef.config["gef.virtualenv_path"] | ||
if path: | ||
activate_script_path = pathlib.Path(path)/"bin"/"activate_this.py" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here path
is already known to be of type pathlib.Path
, no neeed for a 2nd allocation,
activate_script_path = path / "bin/activate_this.py"
assert activate_script_path.is_file()
@@ -9854,6 +9854,9 @@ def __init__(self) -> None: | |||
plugins_dir = GefSetting("", str, "Autoload additional GEF commands from external directory", hooks={"on_write": [GefSetting.no_spaces, ]}) | |||
plugins_dir.add_hook("on_changed", [lambda _, new_val: GefSetting.must_exist(new_val), lambda _, new_val: self.load_extra_plugins(new_val), ]) | |||
gef.config["gef.extra_plugins_dir"] = plugins_dir | |||
venv_path = GefSetting("", str, "Path to the virtualenv used by GEF", hooks={"on_write": [GefSetting.no_spaces, ]}) | |||
venv_path.add_hook("on_changed", [lambda _, new_val: GefSetting.must_exist(new_val), lambda _, new_val: self.load_virtualenv(new_val), ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why we do it above, but you shuld be able to just add on_changed
as an extra key in the hooks
arg for the constructor.
@@ -9943,6 +9946,13 @@ def load_plugins_from_directory(plugin_directory: pathlib.Path): | |||
dbg(f"Loading extra plugins from directory={directory}") | |||
return load_plugins_from_directory(directory) | |||
|
|||
|
|||
def load_virtualenv(self, new_path: Optional[pathlib.Path] = None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this makes sense as a method here
gdb.execute(f"gef config gef.virtualenv_path {self.venv_path}") | ||
|
||
res = gdb.execute("pi __import__('numpy').test()", to_string=True) | ||
assert 'NumPy version' in res |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double quotes
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. You can reopen it by adding a comment to this issue. |
Assigning to shush state-bot |
@ValekoZ Update on this PR? |
Add a
gef.virtualenv_path
config var, and activate the provided virtualenv at startup.This could be used by gef-extras to install python dependencies cleanly.